Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

model: set mtime of links to epoch to prevent spurious rebuilds #1808

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Nov 9, 2021

Description of changes:

We want to rebuild the model package when changing variants, but before
this change, we rebuild every time.  This happens because cargo detects
that the timestamps of the variant-sensitive links we create (and that
we tell it to watch) don't match its output file.  We don't control
creation of the output file, so they'll never match.  Each rebuild
changes the links again, causing another rebuild.

This change sets the mtime of the links to epoch, effectively telling
cargo that they haven't changed.  We still rebuild if the variant
changes, or if someone changes the links behind our back, which would
result in a different mtime.

If you're curious, you can figure this kind of thing out like this:

$ VARIANT=aws-k8s-1.19 \
   CARGO_LOG=cargo::core::compiler::fingerprint=info \
   cargo build
...
INFO  cargo::core::compiler::fingerprint] stale: changed "/home/tjk/work/bottlerocket/sources/models/src/variant/current"
...

Testing done:

  • Normal cargo build works, another normal cargo build doesn't rebuild, yay.
  • Changing VARIANT causes a rebuild; another build with that VARIANT doesn't rebuild, yay.
  • Probably the biggest quality of life improvement for me: cargo running apiclient multiple times doesn't rebuild every time.
  • Created a k8s AMI, tested fine; created an ECS AMI, tested fine; no model caching issues.
  • Confirmed that I could change the link behind our back, set its mtime to epoch, and work around the check if I really wanted to, which I hope no one does.

Performance effects:

  • Obviously, local rebuilds of API-related crates are infinitely faster because they don't rebuild unnecessarily.
  • Doing touch os.spec and rebuilding the os package shows that the initial build takes ~3.5 minutes, as expected, but repeat attempts only take ~52 seconds since cargo knows all the Rust code is already built and it can skip right to license scanning and whatnot.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

We want to rebuild the model package when changing variants, but before
this change, we rebuild every time.  This happens because cargo detects
that the timestamps of the variant-sensitive links we create (and that
we tell it to watch) don't match its output file.  We don't control
creation of the output file, so they'll never match.  Each rebuild
changes the links again, causing another rebuild.

This change sets the mtime of the links to epoch, effectively telling
cargo that they haven't changed.  We still rebuild if the variant
changes, or if someone changes the links behind our back, which would
result in a different mtime.
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍔

@tjkirch tjkirch merged commit ba0571c into bottlerocket-os:develop Nov 10, 2021
@tjkirch tjkirch deleted the build-once-thanks branch November 10, 2021 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants